-
Notifications
You must be signed in to change notification settings - Fork 930
fix(background-agent): fix task completion detection for background sessions #592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(background-agent): fix task completion detection for background sessions #592
Conversation
…essions Background tasks were never completing because: 1. client.session.status() doesn't return background sessions 2. The early `continue` on line 455 skipped ALL completion logic 3. sessionStatus.type access threw when sessionStatus was undefined Changes: - Remove early `continue` when session not found in status - Use optional chaining `sessionStatus?.type` for safe access - Add stability detection: complete when message count unchanged for 3 polls This allows background tasks to complete via message-based detection when session.status() doesn't report them. Tested: explore agent 10s, librarian agent 6s (was hanging indefinitely)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
All contributors have signed the CLA. Thank you! ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Fixed critical bug where background tasks never completed because session.status() doesn't return background sessions. The fix removes the early continue that skipped completion detection and adds message-based stability detection as a fallback.
Key Changes:
- Removed early
continueon line 455 when session not found in status response - Added optional chaining
sessionStatus?.typeto prevent accessing undefined - Implemented stability detection: marks task complete when message count unchanged for 3 consecutive polls (6 seconds)
Issues Found:
- Critical: Tasks that complete with 0 messages will never trigger stability detection (line 513 checks
currentMsgCount > 0) - Type Safety:
stableCountandlastMsgCountproperties added toprogressobject aren't defined inTaskProgresstype
Testing Concern:
Per the project's TDD requirements in AGENTS.md, this bug fix should have accompanying tests. The existing manager.test.ts doesn't cover the pollRunningTasks() method or the new stability detection logic.
Confidence Score: 2/5
- This PR fixes a real bug but introduces a logical flaw that could cause tasks to hang indefinitely
- Score reflects a correct diagnosis of the root cause and appropriate fix approach, but contains a critical edge case bug (line 513: tasks with 0 messages never complete) and violates type safety. Additionally, violates TDD requirements from AGENTS.md by not including tests for the new stability detection logic.
- Pay close attention to the stability detection logic on line 513 - the
currentMsgCount > 0check will cause tasks with no messages to hang forever
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/features/background-agent/manager.ts | 3/5 | Fixed background task completion detection by removing early continue and adding stability detection, but stability detection may fail for tasks with no messages |
Sequence Diagram
sequenceDiagram
participant Manager as BackgroundManager
participant Client as OpencodeClient
participant Session as Background Session
Note over Manager: pollRunningTasks() called every 2s
Manager->>Client: session.status()
Client-->>Manager: allStatuses (background sessions NOT included)
alt Session NOT in status response
Note over Manager: sessionStatus = undefined
Note over Manager: Fall through (no early continue)
Manager->>Client: session.messages({id: sessionID})
Client-->>Manager: messages array
Note over Manager: Count messages.length
alt Message count unchanged for 3 polls (6s)
Manager->>Manager: stableCount >= 3
Manager->>Manager: Mark task as completed
Manager->>Manager: notifyParentSession()
else Message count changed
Manager->>Manager: Reset stableCount to 0
Note over Manager: Continue polling
end
else Session in status response
Note over Manager: sessionStatus defined
alt sessionStatus.type === "idle"
Manager->>Client: session.todo({id: sessionID})
Client-->>Manager: todos array
alt Has incomplete todos
Note over Manager: Continue polling
else No incomplete todos
Manager->>Manager: Mark task as completed
Manager->>Manager: notifyParentSession()
end
else sessionStatus.type !== "idle"
Note over Manager: Task still running, continue polling
end
end
| // Stability detection: if message count unchanged for 3 polls, consider complete | ||
| const currentMsgCount = messages.length | ||
| const progress = task.progress as { stableCount?: number; lastMsgCount?: number } | ||
| if (progress.lastMsgCount === currentMsgCount && currentMsgCount > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Task that completes instantly with 0 messages will never reach stability detection
| if (progress.lastMsgCount === currentMsgCount && currentMsgCount > 0) { | |
| if (progress.lastMsgCount === currentMsgCount) { |
|
|
||
| // Stability detection: if message count unchanged for 3 polls, consider complete | ||
| const currentMsgCount = messages.length | ||
| const progress = task.progress as { stableCount?: number; lastMsgCount?: number } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stableCount and lastMsgCount not in TaskProgress type - extend the type in types.ts to include these fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file
Confidence score: 3/5
- Instant-completion background tasks in
src/features/background-agent/manager.tswill hang because stability detection is blocked by thecurrentMsgCount > 0guard, so the reported change can’t successfully complete those tasks. - Given the high confidence and user-facing impact of the hang, this PR carries some regression risk unless the guard is removed or adjusted.
- Pay close attention to
src/features/background-agent/manager.ts- stability detection never triggers for zero-message tasks.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/features/background-agent/manager.ts">
<violation number="1" location="src/features/background-agent/manager.ts:513">
P1: Tasks that complete instantly with 0 messages will never reach stability detection because of the `currentMsgCount > 0` condition. This causes such tasks to hang indefinitely. Remove the `currentMsgCount > 0` check to allow stability detection for all cases.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
51c587d to
edd9d9f
Compare
|
I have read the CLA Document and I hereby sign the CLA |
edd9d9f to
51c587d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/features/background-agent/manager.ts">
<violation number="1" location="src/features/background-agent/manager.ts:513">
P1: Removing `currentMsgCount > 0` can cause premature task completion. If the background task hasn't generated any messages yet (e.g., network delay, slow agent startup), the stability detection will trigger after 3 polls with 0 messages, incorrectly marking the task as complete before it starts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
- Remove client.session.prompt() notification that interrupted parent sessions - Keep toast notification for subtle UI feedback - Retain completed tasks for 5 minutes for background_output retrieval - Fix formatTaskResult to match sync pattern (sort by time, no optional chaining) The 200ms deletion was too fast - tasks were deleted before output could be fetched. 5-minute retention is safe: DCP operates on LLM context not JS heap, and pruneStaleTasksAndNotifications only cleans after TASK_TTL_MS (30 min).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/features/background-agent/manager.ts">
<violation number="1" location="src/features/background-agent/manager.ts:349">
P1: Potential double-release of concurrency slot. After releasing concurrency, `task.model` should be cleared to prevent other handlers (`session.deleted`, `pruneStaleTasksAndNotifications`) from releasing again during the 5-minute retention period.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
During 5-minute retention period, other handlers (session.deleted, pruneStaleTasksAndNotifications) could release the same concurrency slot again. Setting task.model = undefined after release prevents this.
|
@sisyphus-dev-ai review this |
Code Review: Background Task Completion Detection Fix✅ Successfully Resolved Issues
|
…tion - Replace currentMsgCount > 0 check with elapsedMs >= MIN_STABILITY_TIME_MS - This fixes two edge cases: 1. Tasks that complete with 0 messages no longer hang forever 2. Slow-starting tasks won't be prematurely marked complete - Add comprehensive test suite for stability detection logic: - Test completion after 3 stable polls with messages - Test slow-starting task protection (under 10s threshold) - Test 0-message task completion after threshold - Test stability counter reset on message count change - Test exact 3-poll requirement - Test stability reset and rebuild scenario Fixes: Code review feedback on stability detection logic flaw
Addressed Code Review FeedbackFixed: Stability Detection Logic FlawThe code review correctly identified that the Root Problem: The original check created an impossible choice:
Solution Implemented: Option 1 (Minimum Time Threshold) const MIN_STABILITY_TIME_MS = 10000 // 10 seconds
if (progress.lastMsgCount === currentMsgCount && elapsedMs >= MIN_STABILITY_TIME_MS) {
// Stability detection only kicks in after minimum time
}This ensures:
Added: Comprehensive Test SuiteAdded 6 new tests for
All 20 tests in |
|
This is not a compelete solution, do not use until either someone else figures out a elegant solution or I figure something out myself |
|
continued in pr#628 |
- Add stability-based completion detection (10s min + 3 stable polls) - Fix message extraction to recognize 'reasoning' parts from thinking models - Switch from promptAsync() to prompt() for proper agent initialization - Remove model parameter from prompt body (use agent's configured model) - Add fire-and-forget prompt pattern for sisyphus_task sync mode - Add silent notification via tool.execute.after hook injection - Fix indentation issues in manager.ts and index.ts Incorporates fixes from: - PR code-yeongyu#592: Stability detection mechanism - PR code-yeongyu#610: Model parameter passing (partially) - PR code-yeongyu#628: Completion detection improvements Known limitation: Thinking models (e.g. claude-*-thinking-*) cause JSON Parse errors in child sessions. Use non-thinking models for background agents until OpenCode core resolves this.
…ion (#638) * fix: background task completion detection and silent notifications - Fix TS2742 by adding explicit ToolDefinition type annotations - Add stability detection (3 consecutive stable polls after 10s minimum) - Remove early continue when sessionStatus is undefined - Add silent notification system via tool.execute.after hook injection - Change task retention from 200ms to 5 minutes for background_output retrieval - Fix formatTaskResult to sort messages by time descending Fixes hanging background tasks that never complete due to missing sessionStatus. * fix: improve background task completion detection and message extraction - Add stability-based completion detection (10s min + 3 stable polls) - Fix message extraction to recognize 'reasoning' parts from thinking models - Switch from promptAsync() to prompt() for proper agent initialization - Remove model parameter from prompt body (use agent's configured model) - Add fire-and-forget prompt pattern for sisyphus_task sync mode - Add silent notification via tool.execute.after hook injection - Fix indentation issues in manager.ts and index.ts Incorporates fixes from: - PR #592: Stability detection mechanism - PR #610: Model parameter passing (partially) - PR #628: Completion detection improvements Known limitation: Thinking models (e.g. claude-*-thinking-*) cause JSON Parse errors in child sessions. Use non-thinking models for background agents until OpenCode core resolves this. * fix: add tool_result handling and pendingByParent tracking for resume/external tasks Addresses code review feedback from PR #638: P1: Add tool_result type to validateSessionHasOutput() to prevent false negatives for tool-only background tasks that would otherwise timeout after 30 minutes despite having valid results. P2: Add pendingByParent tracking to resume() and registerExternalTask() to prevent premature 'ALL COMPLETE' notifications when mixing launched and resumed tasks. * fix: address code review feedback - log messages, model passthrough, sorting, race condition - Fix misleading log messages: 'promptAsync' -> 'prompt (fire-and-forget)' - Restore model passthrough in launch() for Sisyphus category configs - Fix call-omo-agent sorting: use time.created number instead of String(time) - Fix race condition: check promptError inside polling loop, not just after 100ms
…ing and sync mode ## Summary This commit represents the culmination of extensive debugging and refinement of the background agent and sisyphus_task systems, building upon changes from PRs code-yeongyu#592, code-yeongyu#610, code-yeongyu#628, code-yeongyu#638, code-yeongyu#648, code-yeongyu#649, code-yeongyu#652, and code-yeongyu#653. ## Investigation Journey ### Initial Problem Background tasks were getting stuck indefinitely. User config model overrides were being ignored for certain agent types. ### Root Cause Analysis 1. Discovered validateSessionHasOutput and checkSessionTodos guards were blocking completion even when tasks had finished 2. Found that sync mode (run_in_background=false) was NOT passing categoryModel to session.prompt(), while async mode was 3. Traced config loading path and found JSONC files weren't being detected 4. Analyzed kdcokenny/opencode-background-agents for reference implementation ### Trial and Error Log **Attempt 1: Add model to resume() in manager.ts** - Hypothesis: Resume needs to pass stored model - Result: REVERTED - PR code-yeongyu#638 intentionally removed this; agent config handles it **Attempt 2: Add userAgents lookup for subagent_type** - Hypothesis: Need to look up agent model from user config - Result: REVERTED - Agent model already applied at creation in config-handler.ts **Attempt 3: Add categoryModel to sync mode prompt** - Hypothesis: Sync mode missing model that async mode passes - Result: SUCCESS - This was the actual bug **Attempt 4: Add debug logging throughout pipeline** - Purpose: Trace model flow through config -> agent -> prompt - Files: 6 files with appendFileSync to omo-debug.log - Result: Confirmed fixes working, then REMOVED all debug logging **Attempt 5: Investigate clickable sessions** - Hypothesis: parentID should make child sessions clickable in UI - Result: parentID IS passed correctly, but sessions don't appear clickable - Analysis: kdcokenny uses same approach; may be OpenCode core limitation - Status: UNRESOLVED - Needs further investigation or OpenCode core change ## Background Agent Completion Detection (PR code-yeongyu#638) Simplified the completion detection logic that was causing tasks to get stuck: - Removed overly complex validateSessionHasOutput and checkSessionTodos guards - Tasks now complete after MIN_IDLE_TIME_MS (5s) elapsed on session.idle event - Added 15-minute global timeout (MAX_RUN_TIME_MS) to prevent runaway tasks - Pattern follows kdcokenny/opencode-background-agents reference implementation ## Model Override Architecture (PRs code-yeongyu#610, code-yeongyu#628, code-yeongyu#638) Established clear separation between category-based and agent-based model handling: | Path | Model Source | |-------------------|-------------------------------------------| | category=X | Explicit from category config (passed) | | subagent_type=X | Agent's configured model (at creation) | | resume | Agent's configured model (not passed) | Key insight from PR code-yeongyu#638: Don't pass model in prompt body for resume/subagent - let OpenCode use the agent's configured model set at creation time in config-handler.ts. ## Sync Mode Category Model Fix (NEW) Fixed critical bug where sync mode (run_in_background=false) with categories was NOT passing the categoryModel to session.prompt(): // BEFORE: Model not passed in sync mode body: { agent: agentToUse, system: systemContent, ... } // AFTER: Model passed when available body: { agent: agentToUse, ...(categoryModel ? { model: categoryModel } : {}), ... } This ensures category model overrides work consistently in both sync and async modes. ## JSONC Config File Support Extended config file detection to support both .json and .jsonc extensions: - getUserConfigDir() now checks for oh-my-opencode.jsonc first - Both cross-platform (~/.config) and Windows (%APPDATA%) paths support JSONC - Enables comments in config files for better documentation ## Test Improvements - Increased sync resume test timeout from 5s to 10s - Test was flaky because timeout = MIN_STABILITY_TIME_MS (race condition) - Added clarifying comments about timing requirements ## Code Cleanup - Removed unused 'os' imports from plugin-config.ts and config-handler.ts - Removed ALL debug logging (hardcoded paths, appendFileSync calls) - Added PR code-yeongyu#638 reference comments for future maintainers ## Verified Test Results (8/8 category + subagent tests pass) | Test | Type | Mode | Result | |-------------------|-------------|-------|--------| | quick | category | async | ✅ | | ultrabrain | category | async | ✅ | | most-capable | category | async | ✅ | | quick | category | sync | ✅ | | librarian | subagent | async | ✅ | | Metis | subagent | async | ✅ | | oracle | subagent | sync | ✅ | | quick + git-master| category | async | ✅ | ## Known Issues & Future Work ### 1. Explore Agent Hangs on Non-Exploration Tasks The explore agent hung when given a simple math query (5+5). This is NOT a regression - explore is a specialized codebase search agent (contextual grep) designed for queries like 'Where is X implemented?' not general Q&A. When given non-exploration tasks, it attempts to search for non-existent patterns and may hang indefinitely due to no max_steps limit. **Recommendation**: Add max_steps: 10 to explore agent config in future PR. ### 2. Clickable Child Sessions Not Working Sessions created via sisyphus_task pass parentID correctly, but don't appear as clickable child sessions in OpenCode's sidebar UI. Investigation showed: - parentID IS being passed to session.create() - kdcokenny/opencode-background-agents uses identical approach - Sessions exist and complete, just not rendered as clickable in UI **Recommendation**: May require OpenCode core change or UI setting discovery. ### 3. Prometheus Agent Correctly Blocked Prometheus (Planner) is a primary agent and correctly rejected when called via sisyphus_task with subagent_type. This is expected behavior - primary agents should only be invoked directly, not via task delegation. ## Files Changed - src/features/background-agent/manager.ts - PR code-yeongyu#638 reference comment - src/tools/sisyphus-task/tools.ts - Sync mode categoryModel fix - src/tools/sisyphus-task/tools.test.ts - Test timeout increase - src/shared/config-path.ts - JSONC extension support - src/plugin-config.ts - Cleanup unused import - src/plugin-handlers/config-handler.ts - Cleanup unused import
- Add stability-based completion detection (10s min + 3 stable polls) - Fix message extraction to recognize 'reasoning' parts from thinking models - Switch from promptAsync() to prompt() for proper agent initialization - Remove model parameter from prompt body (use agent's configured model) - Add fire-and-forget prompt pattern for sisyphus_task sync mode - Add silent notification via tool.execute.after hook injection - Fix indentation issues in manager.ts and index.ts Incorporates fixes from: - PR code-yeongyu#592: Stability detection mechanism - PR code-yeongyu#610: Model parameter passing (partially) - PR code-yeongyu#628: Completion detection improvements Known limitation: Thinking models (e.g. claude-*-thinking-*) cause JSON Parse errors in child sessions. Use non-thinking models for background agents until OpenCode core resolves this.
Summary
Fixes background task completion detection and output retrieval.
Issues Fixed
1. Background sessions not detected (original issue)
client.session.status()doesn't return background sessions - only regular sessionscontinuethat skipped ALL completion logic whensessionStatuswas undefined2. Tasks deleted before output could be retrieved (new fix)
notifyParentSession()deleted tasks after 200msbackground_outputwould fail with "Task was deleted"3. Silent notifications (new fix)
client.session.prompt()that injected "[BACKGROUND TASK COMPLETED]" messages4. formatTaskResult mismatch (new fix)
Changes
manager.ts:
continuewhen session not found - fall through to message-based detectionsessionStatus?.typefor safe property accessclient.session.prompt()notification - keep only toasttools.ts:
formatTaskResultto match sync pattern fromcall-omo-agent/tools.tsTesting
Before fix:
exploreagent: Hung indefinitely (never completed)librarianagent: Hung indefinitelybackground_output: Returned "Task was deleted"After fix:
exploreagent: Completed in ~10 secondslibrarianagent: Completed in ~6 secondsbackground_output: Returns actual agent outputTechnical Details
Stability detection: Tracks
messages.lengthacross polls. When count stays the same for 3 polls (6 seconds at 2s interval), task is complete.5-minute retention safety:
pruneStaleTasksAndNotifications()only prunes after TASK_TTL_MS (30 minutes)status !== "running")Why agents take multiple minutes (not a bug):